-
Notifications
You must be signed in to change notification settings - Fork 352
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make the Voucher view more comforming to designs #5165
Make the Voucher view more comforming to designs #5165
Conversation
IOS-311 Nonconformant design voucher submission dialog in new account flow
The bottoms on the bottom of the voucher redemption screen in the new account flow are misaligned - the bottom margin seems to be too large after dismissing the keyboard. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The margins in compact mode are not correct, they should be aligned with alert margins. See https://mullvad.slack.com/archives/C052D9X882U/p1694419482421749 for discussion and https://reviewable.io/reviews/mullvad/mullvadvpn-app/5152 for a possible solution.
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet)
ios/MullvadVPN/UI appearance/UIMetrics.swift
line 133 at r1 (raw file):
static let padding4: CGFloat = 4 static let padding8: CGFloat = 8 static let padding10: CGFloat = 10
This isn't a criticism of the newly added padding10 but rather of the whole idea of having a corresponding number for each conceivable padding. This will not contribute to a layout with standardised margins and padding, but rather a more cumbersome free-for-all.
ios/MullvadVPN/View controllers/RedeemVoucher/RedeemVoucherContentView.swift
line 44 at r1 (raw file):
comment: "" ) label.textColor = .white
No need to disable translatesAutoresizingMaskIntoConstraints explicitly. This happens automatically when view is added to a stack view. Same goes for all the other views below.
ios/MullvadVPN/View controllers/RedeemVoucher/RedeemVoucherContentView.swift
line 52 at r1 (raw file):
private let enterVoucherLabel: UILabel = { let label = UILabel() label.font = .systemFont(ofSize: 15, weight: .semibold)
Why not use preferredFont here? (and everywhere) We even have an extension with support for weight.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @rablador)
ios/MullvadVPN/UI appearance/UIMetrics.swift
line 133 at r1 (raw file):
Previously, rablador (Jon Petersson) wrote…
This isn't a criticism of the newly added padding10 but rather of the whole idea of having a corresponding number for each conceivable padding. This will not contribute to a layout with standardised margins and padding, but rather a more cumbersome free-for-all.
I agree, but that's what the designs specify, and I didn't want to introduce a magic number
ios/MullvadVPN/View controllers/RedeemVoucher/RedeemVoucherContentView.swift
line 44 at r1 (raw file):
Previously, rablador (Jon Petersson) wrote…
No need to disable translatesAutoresizingMaskIntoConstraints explicitly. This happens automatically when view is added to a stack view. Same goes for all the other views below.
That's good to know, I'll modify those
ios/MullvadVPN/View controllers/RedeemVoucher/RedeemVoucherContentView.swift
line 52 at r1 (raw file):
Previously, rablador (Jon Petersson) wrote…
Why not use preferredFont here? (and everywhere) We even have an extension with support for weight.
Because I couldn't get it to look like the designs. However, I can do this
label.font = .preferredFont(forTextStyle: .body, weight: .semibold).withSize(15)
Which is visually the same as systemFont
so I'll go with that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 7 of 7 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)
ios/MullvadVPN/View controllers/RedeemVoucher/RedeemVoucherContentView.swift
line 280 at r2 (raw file):
private func configureUI() { addConstrainedSubviews([scrollView]) { scrollView.pinEdgesToSuperviewMargins(.all(configuration.layoutMargins))
scrollView.pinEdgesToSuperviewMargins(.all(configuration.layoutMargins))
will fix the last layout margin inconsistency in compact mode.
b6693f6
to
4d76e35
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)
feb1f52
to
55263e1
Compare
This PR makes the voucher view closer to the originally intended design.
A title has been added, a switch for a
compactMode
where the view is now shown fully (when presented from the account view)Fonts and font weights have been changed, as well as some margins.
This change is